Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #283 +/- ##
=======================================
Coverage 90.55% 90.55%
=======================================
Files 69 69
Lines 2414 2414
=======================================
Hits 2186 2186
Misses 228 228 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
GDYendell
left a comment
GDYendell
left a comment
There was a problem hiding this comment.
This is pretty impressive, but it would be a lot of work before being able to merge this.
| - Simple scalar values only | ||
|
|
||
| **Use PVA only** when: | ||
| - Only modern clients (Phoebus) |
There was a problem hiding this comment.
Not sure what you intend by this comment?
There was a problem hiding this comment.
It suggests that Phoebus is PVA only
There was a problem hiding this comment.
I don't agree! It is saying if you are only using modern clients then you can choose to use PVA only.
| # Connection lost - will try to reconnect | ||
| logger.error("Device connection lost") | ||
| raise | ||
| ``` |
There was a problem hiding this comment.
It seems to have figured out that raising in an update loop stops it, but then asserts that the connection error arm will reconnect, which it will not.
Overall this doesn't make a huge amount of sense.
There was a problem hiding this comment.
I'll address this in the next pass
|
|
||
| assert attr.get() == 23.5 | ||
| mock_connection.send_query.assert_called_once_with("TEMP?\r\n") | ||
| ``` |
There was a problem hiding this comment.
This is not bad, but it is just a copy of a FastCS test and not something I would test in a specific driver.
There was a problem hiding this comment.
In the next pass I'll review the working driver it has made and include some tests from that
| - **Learn about dynamic drivers**: See [](dynamic-drivers.md) for runtime device introspection | ||
| - **Explore other transports**: Add Tango, GraphQL, or REST alongside EPICS | ||
| - **Implement methods**: Use `@command` and `@scan` decorators for complex operations | ||
| - **Read the architecture explanation**: Understand how FastCS works under the hood |
There was a problem hiding this comment.
We should ask it to write this
There was a problem hiding this comment.
I was thinking the same thing on the way to work - I will make it happen.
gilesknap
left a comment
gilesknap
left a comment
There was a problem hiding this comment.
Thanks for the feedback. I have implemented all obvious changes and will come back to the others after I have refined the 'stage 1' fastcs-zebra code.
gilesknap
left a comment
gilesknap
left a comment
There was a problem hiding this comment.
I think we should maybe not waste any more time on this for now, given your comments this morning. I had hoped that we would be able to decide on abandoning it with less effort but like you said it was worth a shot.
|
|
||
| # Or in the devcontainer | ||
| python my_device.py | ||
| ``` |
There was a problem hiding this comment.
Gah I thought I deleted the devcontainer bit.
2 participants
No description provided.